A tz property is defined below

Why is this change needed? (just curious to understand)

I am fairly certain that the version currently here is a mistake, since it is overriden by the property definition of tz below. _freq and _tz are set in _simple_new, so I think the idea of also defining them on the class is to make introspecting what attributes exist easy.

This is another we'll-need-this-later (when moving from inheritance to composition)

I'm curious, what is this? Is it intended to be public? It's not present in the public API for 0.23.4

The stdlib datetime.datetime comparison methods check if this attribute exists and if so return NotImplemented, otherwise raise TypeError for non-datetime objects

Just moving this out of the Constructors section for readability

This will be done in the forthcoming PR mentioned in the OP.

why do you need to turn into an object array here? to_datetime handles all of these cases

You're right we could make do without it. I like doing this explicitly because to_datetime is already overloaded and circular.

this is horribly inefficient and unnecessary

If we don't do it here, to_datetime is going to do this. It may be unnecessary, but it is not horribly inefficient. What is a code smell is the circularity involved in calling to_datetime.

then just call array_to_datetime and don’t force the conversion to array

So is the root problem (referenced in your "circularity" comment, and down below in TimedeltaIndex.__new__) that to_datetime / to_timedelta returns an Index instead of an EA?

Could we have the public to_datetime just be a simple

array = _to_datetime(...)
return DatetimeIndex(array)

so the internal _to_datetime returns the array?

So is the root problem (referenced in your "circularity" comment, and down below in TimedeltaIndex.new) that to_datetime / to_timedelta returns an Index instead of an EA?

It's not the fact that it's an Index so much as that it is a circular dependency. I think I can resolve this in an upcoming commit.

Looking through to_datetime and _convert_listlike_datetimes, I don't see a conversion to ndarray[object].

_convert_listlike_datetimes calls ensure_object.

Sorry, to_datetime has in intermediate datetime64[ns] -> object -> datetime64[ns] conversion? That seems unnecessary.

Not sure what you're referring to. As implemented _from_sequence is specifically for list, tuple, or object-dtype NDArray/Index. datetime64-dtype goes through a different path.

_convert_listlike_datetimes calls ensure_object.

That's after an

    # these are shortcutable
    if is_datetime64tz_dtype(arg):
        if not isinstance(arg, DatetimeIndex):
            return DatetimeIndex(arg, tz=tz, name=name)
        if tz == 'utc':
            arg = arg.tz_convert(None).tz_localize(tz)
        return arg

    elif is_datetime64_ns_dtype(arg):
        if box and not isinstance(arg, DatetimeIndex):
            try:
                return DatetimeIndex(arg, tz=tz, name=name)
            except ValueError:
                pass

        return arg

So those both avoid conversion to object.

ExtensionArray._from_sequence is for any sequence of scalar objects, including a ndarray with a specialized type like datetime64[ns]. It'll be used, for example, in factorize(ExtensionArray).

@TomAugspurger thank you for clarifying; I was under the mistaken impression that it was specifically list/tuple/object-dtype.

Are there any restrictions on kwargs that can be added to it? In particular I'm thinking of freq and tz

same

This doesn't call to_timedelta, so this does require that we pass an object array.

then it should
let’s not reinvent the wheel

No, it shouldn't. to_timedelta will just end up calling array_to_timedelta64 like this does, but only after doing a bunch of unecessary dtype checks.

Besides this is what TimedeltaIndex.__new__ currently calls

AssertionError no?

When called from _simple_new this is internal so AssertionError would make sense, but it is also called from __new__ so is in principle user-facing.

Either way I need to add tests for this.

well this should never happen all conversions should be before this

so it should assert

dtype is part of the signature of TimedeltaArray.__new__, which is/will be user-facing. If the user passes the wrong dtype, its a ValueError.

no my point is there should be and i think currently there is already conversion

if it’s wrong at this point it’s not a user error but an incorrect path taken

My point is that this check function is called two times, one of which is the very first thing in TimedeltaArray.__new__.

Apart from the discussion above, is it worth having a 15 line function (including docstrings :-)), for a 2-liner used in two places?
I would maybe simply leave it in place how it was, I think reading something like assert dtype == _TD_DTYPE in TimedeltaArray._simple_new is clearer than calling into a helper function

Reasonable. But hey, its a nice docstring.

This check is made unnecessary by the tz = dtl.validate_tz_from_dtype(dtype, tz) above (implemented a couple PRs ago)

Nope, this isn't quite right. Will revert/fix.

IIUC, then yes you're OK setting copy=False here. By definition, the conversion to datetime64[ns] will involve a copy.

Further question: it is not (yet) possible to simply remove this case? (eventually we should not call the DatetimeArray constructor with an array-like of scalars)

it is not (yet) possible to simply remove this case?

Not if we want to share the extant arithmetic tests (which we do)

(eventually we should not call the DatetimeArray constructor with an array-like of scalars)

I don't share this opinion, would prefer to delay this discussion until it is absolutely necessary.

I don't share this opinion,

Then please raise this in the appropriate issue, as we have been discussing this before (I think it is #23212, although there is probably some more scattered discussion on other related PRs)

would prefer to delay this discussion until it is absolutely necessary.

It is here that you are redesigning the constructors for the array refactor, IIUC, so if there is a time we should discuss it, it is now I think?

Not if we want to share the extant arithmetic tests (which we do)

Can you clarify this a little bit? At what point do the arithmetic tests need to deal with array of objects?
Eg for boxing the constructed values into Series/Index/Array, there a properly dtyped array can be used?

Can you clarify this a little bit? At what point do the arithmetic tests need to deal with array of objects?

The pertinent word here is "extant". Many of the tests in tests/arithmetic pass a list into tm.box_expected or klass.

Ignoring the tests for a moment, I thought we were all on board with the goal of the DatetimelikeArray.__init__ being no inference and no copy.

Back to the tests, it looks like you can you add an entry to box_expected for DatetimeArray to return expected = DatetimeArray._from_sequence(expected)?

Ignoring the tests for a moment, I thought we were all on board with the goal of the DatetimelikeArray.init being no inference and no copy.

My comment to Joris below about mothballing this conversation applies. But short answer is no: I did not get on board with that.

DatetimeArrayMixin -> cls?

And you don't need to get the tz in this case?

DatetimeArrayMixin -> cls?

No. For the moment we are still using inheritance, so this would mess up for DatetimeIndex == DatetimeArray. When we change to composition this check will have to become isinstance(values, (DatetimeArray, ABCDatetimeIndex))

Looking through to_datetime and _convert_listlike_datetimes, I don't see a conversion to ndarray[object].

So is the root problem (referenced in your "circularity" comment, and down below in TimedeltaIndex.__new__) that to_datetime / to_timedelta returns an Index instead of an EA?

Could we have the public to_datetime just be a simple

array = _to_datetime(...)
return DatetimeIndex(array)

so the internal _to_datetime returns the array?

Because of (lack of) caching? This comment makes it seems like it's slower in general, when (if it's caching) it's just slower on repeated use).

BTW (as mentioned elsewhere), I am not sure we should add them as public methods. If we do so, we should add them to all our EAs, or actually even to the EA interface, and not only to TimedeltaArray (or datetimelike arrays).

If we do so, we should add them to all our EAs, or actually even to the EA interface

I'm not necessarily opposed to this, but this isn't obvious to me.

Because of (lack of) caching? This comment makes it seems like it's slower in general, when (if it's caching) it's just slower on repeated use).

Because the Index version defines monotonic_increasing, monotonic_decreasing, and is_unique in a single call via _engine.

I'm curious, what is this? Is it intended to be public? It's not present in the public API for 0.23.4

Ah, this is kinda an API change (raising a TypeError instead of a ValueError).

Seems fine from a consistency P.O.V., but deserves a release note in the API breaking changes section.

I think currently all public cases raise ValueError, so keeping it on that would not give an API change?
(I agree that TypeError is slightly more appropriate though)

My preference would have been to keep it a ValueError and change to a TypeError in a separate PR, but here we are... will add a note in Breaking Changes.

then we should also get the freq from the values as a "cheap" inference? Or would there be cases were an inferred frequency can be different than the actual frequency?

then we should also get the freq from the values as a "cheap" inference?

That's what I'm thinking, yah

Further question: it is not (yet) possible to simply remove this case? (eventually we should not call the DatetimeArray constructor with an array-like of scalars)

I would get out the _values, and then treat that the same as directly passing a DatetimeIndex/DatetimeArray ?

I'll see if there is a graceful way to do this in the next pass (if I ever manage to catch up with all these comments!)

And you don't need to get the tz in this case?

I think currently all public cases raise ValueError, so keeping it on that would not give an API change?
(I agree that TypeError is slightly more appropriate though)

Why is this change needed? (just curious to understand)

I don't think we should replace the handling of object dtype with TimedeltaArray constructor (this should not be able to handle object dtype eventually), but if needed that is fine to leave for a later PR that actually does the split / cleans up the TimedeltaArray constructor

I'm on board with the "leave for later" part of this

This can be a test in the EA base tests.

Although, thinking about it now, I am not sure we should require implementors to handle this case, as the method should never be called with 2D data to begin with.

What is the reason you added handling of this to that method?